Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: fix coverage reporting #20035

Closed
wants to merge 2 commits into from
Closed

lib: fix coverage reporting #20035

wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: #19912
Refs: #19524

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: nodejs#19912
Refs: nodejs#19524
@addaleax addaleax added build Issues and PRs related to build files or the CI. dont-land-on-v6.x labels Apr 14, 2018
@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Apr 14, 2018
@addaleax addaleax added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 14, 2018
@addaleax
Copy link
Member Author

@richardlau
Copy link
Member

CI failure looks related, e.g., https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1010/17833/consoleFull

npm ERR! weird error structured-stack:1
npm ERR! weird error function() {
npm ERR! weird error         ^
npm ERR! weird error 
npm ERR! weird error SyntaxError: Unexpected token (
npm ERR! weird error     at new Script (vm.js:74:7)
npm ERR! weird error     at createScript (vm.js:246:10)
npm ERR! weird error     at runInNewContext (vm.js:291:10)
npm ERR! weird error     at isInsideNodeModules (internal/util.js:393:26)
npm ERR! weird error     at showFlaggedDeprecation (buffer.js:149:8)
npm ERR! weird error     at new Buffer (buffer.js:174:3)
npm ERR! weird error     at Object.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/deps/npm/node_modules/mississippi/node_modules/duplexify/index.js:6:20)
npm ERR! weird error     at Module._compile (internal/modules/cjs/loader.js:678:30)
npm ERR! weird error     at Object.Module._extensions..js (internal/modules/cjs/loader.js:689:10)
npm ERR! weird error     at Module.load (internal/modules/cjs/loader.js:589:32)
make[2]: *** [tools/doc/node_modules/js-yaml/package.json] Error 1
make[1]: *** [doc-only] Error 2
make[1]: *** Waiting for unfinished jobs....

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails CI. Feel free to dismiss my review if I'm not around when this is addressed.

@@ -390,17 +390,16 @@ function isInsideNodeModules() {
// Use `runInNewContext()` to get something tamper-proof and
// side-effect-free. Since this is currently only used for a deprecated API,
// the perf implications should be okay.
getStructuredStack = runInNewContext('(' + function() {
getStructuredStack = runInNewContext(`function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems a leading ( has been accidentally removed.

@addaleax
Copy link
Member Author

@trivikr
Copy link
Member

trivikr commented Apr 15, 2018

The next coverage will start in ~10 hours at 0300 UTC
This PR is being fast-tracked and has got 5 approvals already. So, it should be good to merge by then.

@addaleax
Copy link
Member Author

Landed in 85e1819

@addaleax addaleax closed this Apr 15, 2018
@addaleax addaleax deleted the fix-cov branch April 15, 2018 21:18
addaleax added a commit that referenced this pull request Apr 15, 2018
Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: #19912
Refs: #19524

PR-URL: #20035
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
jasnell pushed a commit that referenced this pull request Apr 16, 2018
Taking the source code of a function and running it in another
context does not play well with coverage instrumentation.
For now, do the simple thing and just write the source code as
a string literal.

Fixes: #19912
Refs: #19524

PR-URL: #20035
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. fast-track PRs that do not need to wait for 48 hours to land. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

coverage.nodejs.org misreporting
10 participants